Conversation
…r.FindType ObjectCreator.FindType passed an inline lambda to ConcurrentDictionary.GetOrAdd that discarded the key parameter ((_)) and captured the local `name` instead. Capturing a local forces the compiler to allocate a new display-class closure and delegate on every call, even on cache hits, which showed up as a significant share of allocations on the deserialization hot path when using a PreresolvingDatumReader. Extract the value factory into a FindTypeUncached(string) method and store a single cached Func<string, Type> delegate (findTypeFactory), created once in the constructor. The factory now uses its `name` parameter (the cache key supplied by GetOrAdd) instead of a captured local, so no closure is allocated per lookup. Behaviour is unchanged. The CA1031 suppression is retargeted to the extracted method.
zcsizmadia
left a comment
zcsizmadia
left a comment
There was a problem hiding this comment.
Thanks, @iemejia — this looks correct to me.
I checked the allocation claim locally (net8.0). Without this change, each cache hit through ObjectCreator.GetType(string, Schema.Type) allocates 96 bytes: the display-class closure plus the delegate. With this change it allocates 0. The refactor otherwise leaves behaviour unchanged. The recursive FindType calls for IList<>/Nullable<> item types still go through the cache as before, and the suppression retarget matches the new member signature.
Storing the delegate in a field is the right approach. Passing the method group FindTypeUncached directly would still allocate a new delegate on every call, because the method is an instance method. Assigning it in the constructor rather than in a field initializer is also necessary, since the initializer can't reference this.
One request: could you add a regression test so this doesn't quietly come back? Something like:
[Test]
public void TestGetTypeCacheHitDoesNotAllocate()
{
var objectCreator = new ObjectCreator();
string name = typeof(Foo).FullName;
// Warm the cache and JIT.
for (int i = 0; i < 100; i++)
{
objectCreator.GetType(name, Schema.Type.Record);
}
long before = GC.GetAllocatedBytesForCurrentThread();
for (int i = 0; i < 10000; i++)
{
objectCreator.GetType(name, Schema.Type.Record);
}
long allocated = GC.GetAllocatedBytesForCurrentThread() - before;
Assert.AreEqual(0, allocated, $"Cache hits allocated {allocated} bytes over 10000 calls");
}It passes on this branch and fails on main with 960000 bytes. GC.GetAllocatedBytesForCurrentThread is available on every test target framework (net6.0/7.0/8.0).
LGTM with or without the test.
2 participants
What changes were proposed in this pull request?
ObjectCreator.FindTypepassed an inline lambda toConcurrentDictionary.GetOrAdd:The lambda discards the key parameter (
(_)) and instead captures the localname. Capturing a local forces the C# compiler to allocate a new display-class closure (and delegate) on every call — including cache hits, since the factory delegate is constructed as an argument regardless of whetherGetOrAddinvokes it. As reported in AVRO-3893, this accounted for ~10% of allocations on a deserialization hot path usingPreresolvingDatumReader.How was this patch fixed?
FindTypeUncached(string name)method.Func<string, Type> findTypeFactorydelegate, created once in the constructor, and pass it toGetOrAdd.nameparameter (the cache key supplied byGetOrAdd) instead of a captured local, so no closure is allocated per lookup.ObjectCreatoris a shared singleton (ObjectCreator.Instance), so the factory delegate is effectively allocated once for the process. Behaviour is unchanged; theCA1031suppression is retargeted fromFindTypeto the extractedFindTypeUncached.How was this patch tested?
dotnet buildofAvro.mainsucceeds with 0 warnings (confirming the retargeted suppression).Avro.testSpecific/ObjectCreator suites pass: 96/96 across net6.0, net7.0, and net8.0.